-
Notifications
You must be signed in to change notification settings - Fork 543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(bzlmod): keep the lockfile platform independent when resolving python #2135
fix(bzlmod): keep the lockfile platform independent when resolving python #2135
Conversation
@fmeum do you have a better idea of how we could effectively do @rickeylev, do you have ideas how we could test this? Should we have one of the |
I don't see a better way at the moment. We should probably add a Cc @Wyverald |
Yeah, that seems fair. |
19698d6
to
777d7ed
Compare
e2b0f00
to
65f6efa
Compare
re: testing: I think --lockfile_mode=error in the example is OK. The other option is an integration test that...I guess just sets --lockfile_mode? If there's something in particular to look for in the output, the python runner can be used to read the lock file and look for that. I think we'll just have to see how it plays out with the downstream bazel testing. If it causes too many bogus failures, then we can factor out a smaller test under a separate CI job that sets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally, looks fine, so approved. Just comments about some doc phrasing and code reuse.
python/uv/private/pin.bzl
Outdated
load("//python:py_binary.bzl", "py_binary") | ||
load("//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED") # buildifier: disable=bzl-visibility | ||
|
||
_REQUIREMENTS_TARGET_COMPATIBLE_WITH = select({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does uv only provide linux/mac binaries...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, windows
binaries are also provided, just the genrule
implementation is in shell, however, I have decided to remove the select here and just enable this in bzlmod
.
Also: - Fix the lock file rule to take into account the currently existing lockfiles. - Use a transition to allow for a smart selection of the python version to generate the lock file for.
Summary: - separate target to update the pip requirements - Use a universal requirements file
…thon Before this PR the lockfile would become platform dependent when the `requirements` file would have env markers. This was not caught because we do not have MODULE.bazel.lock checked into the `rules_python` repository because the CI is running against many versions and the lock file is different, therefor we would not be able to run with `bazel build --lockfile_mode=error`. With this change we use the label to `BUILD.bazel` which is living next to the `python` symlink and since the `BUILD.bazel` is the same on all platforms, the lockfile will remain the same. Work towards bazelbuild#1105, bazelbuild#1868.
- Improve the CHANGELOG wording. - Rename `pin` to `lock` and set the load visibility to internal. - Retain the same naming convention of `requirements.update`. - Use `py_binary` with the transitions. - Add a simple validation to the `mrctx.watch` wrapping.
05e8f10
to
31c82ba
Compare
Before this PR the lockfile would become platform dependent when the
requirements
file would have env markers. This was not caught becausewe do not have MODULE.bazel.lock checked into the
rules_python
repository because the CI is running against many versions and the lock
file is different, therefore we would not be able to run with
bazel build --lockfile_mode=error
.With this change we use the label to
BUILD.bazel
which is living nextto the
python
symlink and since theBUILD.bazel
is the same on allplatforms, the lockfile will remain the same.
Summary
This breaks the CI, because the python interpreter file hash is added to the lock file.
Fixes #1105 and #1868 for real this time.
Implements an additional helper for #1975.